feat: Added support for Claude 3+ Chat API in Bedrock #2870

RyanKadri · 2025-01-10T20:40:35Z

Description

The current version of the Bedrock instrumentation is mostly geared toward supporting the Claude Text Completions API but may not split messages as expected in multi-turn conversations with the Messages API.

The Text Completions API supports a single prompt message but the Messages API, lets you provide multiple distinct messages (context and prompt) to the model. Currently the agent combines those separate context messages into a single prompt but it probably should follow the pattern we use in OpenAI instrumentation of splitting them into multiple LlmChatCompletionMessage events (still one LlmChatCompletionSummary per API call)

To make things more complicated, the Messages API also allows messages to have sub-chunks to support multi-modal use-cases (Anthropic docs for how this works). For example, one conceptual multi-modal message might be What is this animal <picture of an elephant> and where does it live?. For these cases, if a single message has multiple chunks, this PR attempts to join those together into one string representation with reasonable placeholders for non-text components. Further down the road, the AI Monitoring team needs to do a bit of work to see if we can represent those non-text chunks as placeholders in a string. For now though, we just want something to show.

How to Test

The AI Monitoring sample app is running this version of the instrumentation for Anthropic models. The AI Monitoring team can help you out with where to see the data it produces

Related Issues

N/A

bizob2828

thanks for the PR @RyanKadri! A few tiny suggestions but you also have merge conflicts so I didn't do a thorough review yet.

bizob2828 · 2025-01-13T15:55:24Z

lib/instrumentation/aws-sdk/v3/bedrock.js

    const chatCompletionMessage = new LlmChatCompletionMessage({
      agent,
      segment,
      bedrockCommand,
      bedrockResponse,
      isResponse: true,
-      index: index + 1,
+      index: index,


Suggested change

index: index,

index,

this is a little confusing. what about splitting vars so the prompt index can be promptIndex and then in here you have completionIndex, then this index is promptIndex + completionIndex

lib/instrumentation/aws-sdk/v3/bedrock.js

bizob2828 · 2025-01-13T20:30:34Z

please run npm run lint:fix to fix your linting errors or address them individually by doing npx eslint --fix path/to/file.js

RyanKadri · 2025-01-13T20:50:22Z

Sounds good 👍. I have a couple other changes incoming for another unit test scenario I want to cover

RyanKadri · 2025-01-14T17:11:41Z

(Looking into unit tests)

bizob2828 · 2025-01-16T17:41:41Z

@RyanKadri is this ready for another review? You have merge conflicts again

RyanKadri · 2025-01-16T17:54:17Z

Sorry. I got caught up in a couple other tasks. Still taking a look at the tests. I'll fix the merge conflicts as well

RyanKadri · 2025-01-16T19:20:36Z

lib/instrumentation/aws-sdk/v3/bedrock.js

+  embeddings.forEach(embedding => {
+    recordEvent({ agent, type: 'LlmEmbedding', msg: embedding })


I need to do a bit more research on this but I think some of the Bedrock embedding models allow you to make a single invoke call that generates several embeddings (see this Cohere blog for an example). So I think it might be correct to allow unrolling one embedding command to several embedding events. Currently all the embedding models in the command class produce a single prompt but I'm wondering if the Cohere one is also incorrectly squashing messages in some cases.

I might try to treat that as a separate PR / task if you're alright with that though

If that's the case, how do you want to handle an error? I assume we still want one error attached to the transaction? I opted to only attach the embedding info if there's one event to keep the current behavior but I'm not sure that's correct

I don't know this enough, but seems ok for now

RyanKadri · 2025-01-16T21:09:31Z

@bizob2828 I think I'm good for another review if you have a sec. I see some versioned tests are failing but they're complaining about an s3 client version and I don't think I touched anything that should break that?

bizob2828

overall looks good, just a few questions around some testing gaps

bizob2828 · 2025-01-16T21:09:12Z

lib/llm-events/aws-bedrock/chat-completion-summary.js

@@ -36,7 +36,7 @@ class LlmChatCompletionSummary extends LlmEvent {
    const cmd = this.bedrockCommand
    this[cfr] = this.bedrockResponse.finishReason
    this[rt] = cmd.temperature
-    this[nm] = 1 + this.bedrockResponse.completions.length
+    this[nm] = (this.bedrockCommand.prompt?.length ?? 0) + this.bedrockResponse.completions.length


based on the bedrockCommand file this is always an array. so if the length is 0 it'll be 0, do we need this defaulting to 0 when prompt.length is falsey?

bizob2828 · 2025-01-16T21:14:56Z

lib/instrumentation/aws-sdk/v3/bedrock.js

+  embeddings.forEach(embedding => {
+    recordEvent({ agent, type: 'LlmEmbedding', msg: embedding })


I don't know this enough, but seems ok for now

bizob2828 · 2025-01-16T21:16:50Z

lib/llm-events/aws-bedrock/bedrock-response.js

@@ -63,7 +65,7 @@ class BedrockResponse {
        // Streamed response
        this.#completions = body.completions
      } else {
-        this.#completions = body?.content?.map((c) => c.text)
+        this.#completions = [stringifyClaudeChunkedMessage(body?.content)]


i see a versioned test but not a unit test for this

bizob2828 · 2025-01-16T21:18:54Z

lib/llm-events/aws-bedrock/bedrock-command.js

-      result = collected.join(' ')
+      return [{ role: 'user', content: this.#body.prompt }]
+    } else if (this.isClaudeMessagesApi() === true) {
+      return normalizeClaude3Messages(this.#body?.messages ?? [])


I don't see a unit nor versioned tests that asserts that this.#body.messages is falsey and defaults to empty array, is this needed? if so, please write a unit test

This was meant to handle the customer passing an invalid body. I realized the default value isn't needed here because I have it handled below anyway. I added a unit test for this case anyway to catch where it's handled lower down

bizob2828 · 2025-01-16T21:23:01Z

@bizob2828 I think I'm good for another review if you have a sec. I see some versioned tests are failing but they're complaining about an s3 client version and I don't think I touched anything that should break that?

I reviewed it. Yea, that s3 failure seems like an issue with npm and installing, it passes locally for me, if it's still failing after i re-ran, i can help dig

codecov · 2025-01-16T21:35:51Z

Codecov Report

Attention: Patch coverage is 98.46154% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.34%. Comparing base (46462d0) to head (340fecb).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/llm-events/aws-bedrock/bedrock-command.js	96.36%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2870      +/-   ##
==========================================
- Coverage   97.39%   97.34%   -0.06%     
==========================================
  Files         308      310       +2     
  Lines       47330    47512     +182     
==========================================
+ Hits        46099    46252     +153     
- Misses       1231     1260      +29

Flag	Coverage Δ
integration-tests-cjs-18.x	`73.02% <24.61%> (-0.16%)`	⬇️
integration-tests-cjs-20.x	`73.03% <24.61%> (-0.15%)`	⬇️
integration-tests-cjs-22.x	`73.05% <24.61%> (-0.16%)`	⬇️
integration-tests-esm-18.x	`49.78% <24.61%> (-0.05%)`	⬇️
integration-tests-esm-20.x	`49.79% <24.61%> (-0.05%)`	⬇️
integration-tests-esm-22.x	`49.83% <24.61%> (-0.05%)`	⬇️
unit-tests-18.x	`88.99% <79.23%> (-0.13%)`	⬇️
unit-tests-20.x	`88.99% <79.23%> (-0.13%)`	⬇️
unit-tests-22.x	`88.98% <79.23%> (-0.14%)`	⬇️
versioned-tests-18.x	`79.27% <95.38%> (-0.12%)`	⬇️
versioned-tests-20.x	`79.27% <95.38%> (-0.12%)`	⬇️
versioned-tests-22.x	`79.28% <95.38%> (-0.12%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bizob2828 · 2025-01-17T20:43:44Z

lib/llm-events/aws-bedrock/bedrock-command.js

+ */
+function normalizeClaude3Messages(messages) {
+  const result = []
+  for (const message of messages ?? []) {


i'm going to approve this but we can follow up on the missing coverage here: messages is falsey and there is an array of messages but one is null

bizob2828 self-assigned this Jan 13, 2025

bizob2828 requested changes Jan 13, 2025

View reviewed changes

bizob2828 changed the title ~~Support Claude 3+ Chat API in Bedrock~~ feat: Added support for Claude 3+ Chat API in Bedrock Jan 13, 2025

RyanKadri force-pushed the claude-3-messages branch from 23c37dc to c31dace Compare January 13, 2025 21:26

RyanKadri added 4 commits January 16, 2025 14:07

feat: Support Anthropic v3 Chat SPI

12c5e31

feat: Unit tests

d41cfab

fix: PR comments

831e5ca

fix: Embedding unit tests

b6b59d0

RyanKadri force-pushed the claude-3-messages branch from c31dace to b6b59d0 Compare January 16, 2025 19:12

RyanKadri commented Jan 16, 2025

View reviewed changes

fix: More unit tests

cf3a875

bizob2828 requested changes Jan 16, 2025

View reviewed changes

fix: PR feedback

340fecb

bizob2828 reviewed Jan 17, 2025

View reviewed changes

bizob2828 approved these changes Jan 17, 2025

View reviewed changes

bizob2828 merged commit 6a83abf into newrelic:main Jan 17, 2025
26 of 27 checks passed

bizob2828 mentioned this pull request Jan 17, 2025

test: Improved test coverage of normalizing claude 3 messages #2893

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added support for Claude 3+ Chat API in Bedrock #2870

feat: Added support for Claude 3+ Chat API in Bedrock #2870

RyanKadri commented Jan 10, 2025 •

edited

Loading

bizob2828 left a comment

bizob2828 Jan 13, 2025

bizob2828 Jan 13, 2025

bizob2828 commented Jan 13, 2025

RyanKadri commented Jan 13, 2025

RyanKadri commented Jan 14, 2025

bizob2828 commented Jan 16, 2025

RyanKadri commented Jan 16, 2025

RyanKadri Jan 16, 2025

RyanKadri Jan 16, 2025

bizob2828 Jan 16, 2025

RyanKadri commented Jan 16, 2025

bizob2828 left a comment

bizob2828 Jan 16, 2025

bizob2828 Jan 16, 2025

bizob2828 Jan 16, 2025

bizob2828 Jan 16, 2025 •

edited

Loading

RyanKadri Jan 17, 2025

bizob2828 commented Jan 16, 2025

codecov bot commented Jan 16, 2025 •

edited

Loading

bizob2828 Jan 17, 2025

		embeddings.forEach(embedding => {
		recordEvent({ agent, type: 'LlmEmbedding', msg: embedding })

feat: Added support for Claude 3+ Chat API in Bedrock #2870

feat: Added support for Claude 3+ Chat API in Bedrock #2870

Conversation

RyanKadri commented Jan 10, 2025 • edited Loading

Description

How to Test

Related Issues

bizob2828 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizob2828 commented Jan 13, 2025

RyanKadri commented Jan 13, 2025

RyanKadri commented Jan 14, 2025

bizob2828 commented Jan 16, 2025

RyanKadri commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanKadri commented Jan 16, 2025

bizob2828 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizob2828 Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizob2828 commented Jan 16, 2025

codecov bot commented Jan 16, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

RyanKadri commented Jan 10, 2025 •

edited

Loading

bizob2828 Jan 16, 2025 •

edited

Loading

codecov bot commented Jan 16, 2025 •

edited

Loading